Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.00%. Comparing base (d5d6f6f) to head (05ddf87).
⚠️ Report is 8 commits behind head on main.

@@            Coverage Diff             @@
##             main    #1372      +/-   ##
==========================================
- Coverage   80.02%   80.00%   -0.03%     
==========================================
  Files          20       20              
  Lines         991      990       -1     
==========================================
- Hits          793      792       -1     
  Misses        198      198              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Not sure why it is showing such a less coverage in cmd/authctl/* files. I have added quite some cases which should hit those lines. Please let me know if I am missing something. Thanks!

Edit: I think the reason is that those tests are building authctl and then running it using exec.Command(..) to test stuff... and not directly calling the go code in tests. But that way of testing is pre existing and I am following the same.

Thanks for the PR!

Note that #640 has a sub-issue (#830) around cleaning up broker-stored user data on deletion. Since it's related, it would be good to address it in the same PR.

Thanks for taking a look @nooreldeenmansour.

I will work on closing #830 as well in the same PR.

Regarding the issues you have highlighted,

When a user is deleted, their home directory is left behind. We should either add a -r/--remove flag (similar to userdel ) to optionally clean it up

Adding a flag to optionally delete the home directory makes sense.

We shouldn't allow deletion of a group that is the primary group of an existing user.

Makes sense. I will take care of this.

When a user is deleted, their primary group isn't cleaned up, leaving the database in an inconsistent state.

I intentionally did it that way because I was unsure about the scenarios where some other user is also manually made part of another users primary group. I would like to hear your thoughts on how do we handle this situation? Should we implement it in a way that the user's primary group deletion happens only when only that user was part of it's group?

Re-login after deletion assigns a new UID, breaking home directory ownership

This is similar to the warning we show with the delete command. Once a user is deleted, its UID can be assigned to another user. Similarly when it is recreated, it might get a new UID. I think we can add the wordings for later in the warning message. Other than that, I couldn't figure out an easy way to handle that. Our warning message already suggests to lock user instead of deleting to make sure the UID stays reserved. Anything else you have in mind to handle this?

I would like to hear your thoughts on how do we handle this situation? Should we implement it in a way that the user's primary group deletion happens only when only that user was part of it's group?

I think it makes sense to handle this the standard UNIX way. As detailed in the userdel:

If USERGROUPS_ENAB is defined to yes in /etc/login.defs, userdel will delete the group with the same name as the user. To avoid inconsistencies in the passwd and group databases, userdel will check that this group is not used as a primary group for another user.

USERGROUPS_ENAB (boolean)
If set to yes, userdel will remove the user's group if it contains no more members.

Other than that, I couldn't figure out an easy way to handle that. Our warning message already suggests to lock user instead of deleting to make sure the UID stays reserved. Anything else you have in mind to handle this?

The warning message is likely sufficient, since it clearly states the UID drift as a potential consequence. I'll wait on @adombeck's suggestion regarding this

I think it makes sense to handle this the standard UNIX way.

That sounds reasonable. For the scenario where a user's primary group has other members as well - as far as I understand, authd can only determine whether some other user belongs to a users primary group if the other user itself is managed by authd (please correct me if that’s not accurate).

Based on that, we could safely delete a user’s primary group when it has no other members, while also issuing a warning that any manually added users in that group would lose their group membership.

@nooreldeenmansour @adombeck WDYT?

P.S. I know you’re busy with the upcoming release, so feel free to respond whenever convenient.

Thank you, @shiv-tyagi, for the PR and thank you @nooreldeenmansour for the excellent review!

Note that #640 has a sub-issue (#830) around cleaning up broker-stored user data on deletion. Since it's related, it would be good to address it in the same PR.

I agree that we should handle the deletion of the broker's user data before merging this PR, because it would be confusing if we still kept user data after running authctl delete user. However, since it requires adding a new API method to the broker, I expect it to be a quite big change itself, so maybe for the sake of having smaller PRs, which we can merge more quickly, it would make sense to do that in a separate PR which we merge before this one.

I would like to hear your thoughts on how do we handle this situation? Should we implement it in a way that the user's primary group deletion happens only when only that user was part of it's group?

I think it makes sense to handle this the standard UNIX way. As detailed in the userdel:

If USERGROUPS_ENAB is defined to yes in /etc/login.defs, userdel will delete the group with the same name as the user. To avoid inconsistencies in the passwd and group databases, userdel will check that this group is not used as a primary group for another user.

USERGROUPS_ENAB (boolean)
If set to yes, userdel will remove the user's group if it contains no more members.

I agree that we should handle it the same way userdel does when usergroups are enabled, but regardless of the value of USERGROUPS_ENAB. I don't see a use case for disabling this behavior in authd (and if there was one, the place to add the setting would be /etc/authd/authd.yaml).

Other than that, I couldn't figure out an easy way to handle that. Our warning message already suggests to lock user instead of deleting to make sure the UID stays reserved. Anything else you have in mind to handle this?

The warning message is likely sufficient, since it clearly states the UID drift as a potential consequence. I'll wait on @adombeck's suggestion regarding this

I agree, a warning message explaining the issue with file ownership and suggesting to disable the user instead is the best we can do.

That sounds reasonable. For the scenario where a user's primary group has other members as well - as far as I understand, authd can only determine whether some other user belongs to a users primary group if the other user itself is managed by authd (please correct me if that’s not accurate).

We don't have to worry about that, because the primary groups only exist in the authd database, not /etc/group, so local users can't be added to those groups (related: #1374).

Hi @adombeck @nooreldeenmansour

When a user is deleted, their home directory is left behind. We should either add a -r/--remove flag (similar to userdel ) to optionally clean it up

I tried implementing this in authd's User service. I am getting error deleting the home directory. When I checked I found that the authd's service is configured in a way that it cannot touch stuff inside /home. Would it be okay to add /home to the list of paths the authd process is allowed to read write?

Edit: the process would also require CAP_DAC_OVERRIDE, I just checked.

When a user is deleted, their home directory is left behind. We should either add a -r/--remove flag (similar to userdel ) to optionally clean it up, or at minimum make the warning message explicit that the home directory is preserved and requires manual cleanup.

Also note that when targeting ubuntu we should also ensure that deluser (as we try to do with adduser) policies are respected too.

When I checked I found that the authd's service is configured in a way that it cannot touch stuff inside /home. Would it be okay to add /home to the list of paths the authd process is allowed to read write?

Yeah, we'll haven to loosen the restrictions on the authd systemd service (see debian/authd.service.in). ReadWritePaths=-/home should work for directories below /home, but home directories could also be elsewhere. We could try allow-list other directories, but maybe we should just remove the filesystem restrictions we currently have, because they don't really provide an effective security boundary anyway. I'll create an issue where we can discuss that.

Yeah, we'll haven to loosen the restrictions on the authd systemd service (see debian/authd.service.in)

Actually, after taking a closer look, I don't see anything in our existing service definition which would prevent writing to /home. Which error do you see? Can you commit the changes so I can try to reproduce it?

Also note that when targeting ubuntu we should also ensure that deluser (as we try to do with adduser) policies are respected too.

Which policies are you referring to, @3v1n0?

@adombeck

I have pushed my changes. This includes the broker side changes as well (I need to organise the commits better).

Actually, after taking a closer look, I don't see anything in our existing service definition which would prevent writing to /home. Which error do you see? Can you commit the changes so I can try to reproduce it?

I checked again, turned out that it is a missing CAP_DAC_OVERRIDE capability in the authd process.
(earlier I was under an impression that not adding /home to ReadWritePaths might have been preventing it, but it looks like just doing that is not enough and if we add the above mentioned capability to the process, later is not even needed.)

I checked again, turned out that it is a missing CAP_DAC_OVERRIDE capability in the authd process.

That makes sense. Then let's add CAP_DAC_OVERRIDE to the CapabilityBoundingSet and explain in the comment above why we need it.

Hey @adombeck! I have submitted #1421 which contains the broker side of changes for user data deletion as suggested by you.

This PR is also ready for another round of review. I have addressed the comments posted by @nooreldeenmansour. Please take a look and feel free to suggest improvements.

TestUserDeleteCommand is failing because I have removed broker side changes from this PR to help with the review. I will rebase when #1421 is merged and they should pass.